Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use privileged client in registry instead of user client when getting or creating signatures #16353

Merged
merged 2 commits into from
Sep 18, 2017

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Sep 14, 2017

@csrwng

Fixes: #16349

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 14, 2017
@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2017
@runcom
Copy link
Member

runcom commented Sep 14, 2017

👍

@runcom
Copy link
Member

runcom commented Sep 14, 2017

/cc @giuseppe

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 14, 2017

@csrwng i was wrong, this should imagestreamimage and not imagesignatures (imagesignatures does not have 'get' method, just create and delete), registry reads the signatures by querying the imagestreamimage

@runcom
Copy link
Member

runcom commented Sep 14, 2017

(for the record we need this fix in 3.7, but I see it's alpha so this should go in anyway /cc @mrunalp)

@csrwng
Copy link
Contributor

csrwng commented Sep 14, 2017

I can confirm that this worked for me on a test cri-o cluster.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 14, 2017

@liggitt || @enj the registry has an signature endpoint that you can use to get signature... this endpoint is proxying the signature data from openshift (signatures are stored in openshift images...).

I believe that granting the image-puller ability to get the imagestreamimage to serve the signature content is fine, but want to double-check if you can think of any security implications.

This is the client call: https://github.com/openshift/origin/blob/master/pkg/dockerregistry/server/signaturedispatcher.go#L156

@@ -533,6 +533,8 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {
Rules: []rbac.PolicyRule{
// pull images
rbac.NewRule("get").Groups(imageGroup, legacyImageGroup).Resources("imagestreams/layers").RuleOrDie(),
// read signatures
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could just add "imagestreamimages" to the resources in the rule above this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanted to make it clear why that rule is there

@enj
Copy link
Contributor

enj commented Sep 14, 2017

the registry has an signature endpoint that you can use to get signature... this endpoint is proxying the signature data from openshift (signatures are stored in openshift images...)

Er so what is preventing us from using that?

I believe that granting the image-puller ability to get the imagestreamimage to serve the signature content is fine, but want to double-check if you can think of any security implications.

So what extra things can image-puller see/do with this permission? I see that it can get the entire image, and not just the signatures. I just do not have enough context to say how much extra information you are leaking to get access to the signature data.

@openshift/sig-security

@@ -533,6 +533,8 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {
Rules: []rbac.PolicyRule{
// pull images
rbac.NewRule("get").Groups(imageGroup, legacyImageGroup).Resources("imagestreams/layers").RuleOrDie(),
// read signatures
rbac.NewRule("get").Groups(imageGroup, legacyImageGroup).Resources("imagestreamimages").RuleOrDie(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this role is intended to grant permissions to call the registry endpoints. the registry is already authorizing getting signatures based on get imagestreams/layers, have the registry's signature endpoint get signatures from the API using the registry's client, not the user's

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 14, 2017
@mfojtik mfojtik changed the title allow image-puller role to read image signatures use privileged client in registry instead of user client when getting or creating signatures Sep 14, 2017
@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 14, 2017

@liggitt tested locally and it is working (it returns empty signatures)

@mfojtik mfojtik added this to the 3.7.0 milestone Sep 14, 2017
client, ok := userClientFrom(s.ctx)
if !ok {
s.handleError(s.ctx, errcode.ErrorCodeUnknown.WithDetail("unable to get origin client"), w)
client, err := RegistryClientFrom(s.ctx).Client()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before we let the user create signatures via the registry, verifyImageSignatureAccess ensures they have create imagesignatures permission, so using the user client here would work. I'm not actually sure whether the registry has create imagesignatures permission by default

client, ok := userClientFrom(s.ctx)
if !ok {
s.handleError(s.ctx, errcode.ErrorCodeUnknown.WithDetail("unable to get origin client"), w)
client, err := RegistryClientFrom(s.ctx).Client()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks fine

@liggitt
Copy link
Contributor

liggitt commented Sep 14, 2017

switching to use the registry client on get looks fine. I'm not sure about the switch for put

@csrwng
Copy link
Contributor

csrwng commented Sep 14, 2017

The code in its current state worked for me on cri-o

@jim-minter
Copy link
Contributor

/unassign jim-minter

s.handleError(s.ctx, errcode.ErrorCodeUnknown.WithDetail("unable to get origin client"), w)
client, err := RegistryClientFrom(s.ctx).Client()
if err != nil {
s.handleError(s.ctx, errcode.ErrorCodeUnknown.WithDetail(fmt.Sprintf("unable to get origin client", err)), w)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the Sprintf miss the formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good catch, thx

s.handleError(s.ctx, errcode.ErrorCodeUnknown.WithDetail("unable to get origin client"), w)
client, err := RegistryClientFrom(s.ctx).Client()
if err != nil {
s.handleError(s.ctx, errcode.ErrorCodeUnknown.WithDetail(fmt.Sprintf("unable to get origin client", err)), w)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@simo5
Copy link
Contributor

simo5 commented Sep 14, 2017

Do we have a test that confirms user's that have no access to adding signtures actually fail to add them ?
If not we MUST (as in RFCs) have such a test as part of this PR.

@liggitt
Copy link
Contributor

liggitt commented Sep 16, 2017

# github.com/openshift/origin/pkg/dockerregistry/server
pkg/dockerregistry/server/signaturedispatcher.go:145: undefined: RegistryClientFrom
[ERROR] PID 1623: hack/lib/build/binaries.sh:228: `GOOS=${platform%/*} GOARCH=${platform##*/} go install -pkgdir "${OS_OUTPUT_PKGDIR}/${platform}" -tags "${OS_GOFLAGS_TAGS-} ${!platform_gotags_envvar:-}" -ldflags="${local_ldflags}" "${goflags[@]:+${goflags[@]}}" "${nonstatics[@]}"` exited with status 2.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2017

@dmage seems like this was broken by #15796 can you please provide an advise how can one get privileged client in signaturedispatcher?

@liggitt
Copy link
Contributor

liggitt commented Sep 17, 2017

pass registryClient to RegisterSignatureHandlers and hold a reference to it in the handler, rather than retrieve it from the context

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2017

@liggitt yeah, already working on that...

@dmage
Copy link
Contributor

dmage commented Sep 17, 2017

@mfojtik make RegisterSignatureHandler and SignatureDispatcher to be methods of (app *App). That will allow you use app.registryClient in SignatureDispatcher. You can put app.registryClient into signatureHandler.

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2017

@dmage done, unfortunately i have to plumb the unit tests as well... puzzled how they ever worked as the signaturedispatcher use the clientset and not openshift client.

@dmage
Copy link
Contributor

dmage commented Sep 17, 2017

@mfojtik don't forget to push your changes :)

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2017

@dmage once they compile...

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 17, 2017
@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2017

@dmage ok, running out of options, but i'm getting signaturedispatcher_test.go:113: unexpected response status: 401 when running the TestSignatureGet, any clues?

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 17, 2017

sunday pebkac, nvmd.

@@ -57,6 +57,8 @@ func TestSignatureGet(t *testing.T) {
t.Fatal(err)
}

os.Setenv("OPENSHIFT_DEFAULT_REGISTRY", "localhost:5000")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmage for some strange reasons this is needed (at least on OSX) otherwise the NewApp() will fail... we should make this a field in openshift middleware config

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix this as part of this PR? This is pretty wonky

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfojtik yes, configuration check is moved to the app constructor. Adding a field in config will introduce yet another one environment variable for this value (we already have DEFAULT_REGISTRY_URL and OPENSHIFT_DEFAULT_REGISTRY). But I like this idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in a separate commit

@@ -74,6 +78,11 @@ func newRepositoryConfig(ctx context.Context, options map[string]interface{}) (r
} else {
context.GetLogger(ctx).Infof("DEPRECATED: %q is deprecated, use the %q instead", DockerRegistryURLEnvVar, OpenShiftDefaultRegistryEnvVar)
}
rc.registryAddr, err = getStringOption(DockerRegistryURLEnvVarOption, "dockerregistryurl", rc.registryAddr, options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if rc.registryAddr has not filled yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would say that having this in configuration file overrides all env vars?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code erases previous value no matter what. And environment variables usually overrides values in a config file.

@@ -15,6 +15,10 @@ const (
// DEPRECATED: Use the OPENSHIFT_DEFAULT_REGISTRY instead.
DockerRegistryURLEnvVar = "DOCKER_REGISTRY_URL"

// DockerRegistryURLEnvVarOption is an optional environment that overrides the
// DOCKER_REGISTRY_URL.
DockerRegistryURLEnvVarOption = "REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_DOCKER_REGISTRY_URL"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/DOCKER_REGISTRY_URL/DOCKERREGISTRYURL/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a second envvar for this?

"storage": {{Name: "openshift"}},
"registry": {{Name: "openshift", Options: configuration.Parameters{"dockerregistryurl": "localhost:5000"}}},
"repository": {{Name: "openshift", Options: configuration.Parameters{"dockerregistryurl": "localhost:5000"}}},
"storage": {{Name: "openshift", Options: configuration.Parameters{"dockerregistryurl": "localhost:5000"}}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this option is only for the repository middleware

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 18, 2017

@dmage comments addressed

@mfojtik
Copy link
Contributor Author

mfojtik commented Sep 18, 2017

/retest

@dmage
Copy link
Contributor

dmage commented Sep 18, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, liggitt, mfojtik

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15834, 16321, 16353, 15298, 15433)

@openshift-merge-robot openshift-merge-robot merged commit e4defee into openshift:master Sep 18, 2017
@mfojtik mfojtik deleted the allow-read-signatures branch September 5, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.